issues/170: Fixed message serial out of sync after recover #175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change might still be wrong. Paddy and I are discussing. |
|
What do you reckon to that Paddy? |
paddybyers
requested changes
Oct 6, 2016
| @@ -271,6 +272,7 @@ public void run() { | |||
| ***************************************/ | |||
|
|
|||
| public void onMessage(ProtocolMessage message) throws AblyException { | |||
| Log.v(TAG, "onMessage(): " + new String(ProtocolSerializer.writeJSON(message))); | |||
There was a problem hiding this comment.
I don't like the way that this serialises every message even though the log level doesn't require it to be emitted. I think it has to be done conditionally on the log level, or the log API has to take a format string and arguments so expansion is under control of the logger.
| * connection, and thus one more than the highest message serial | ||
| * in the queue. | ||
| */ | ||
| public synchronized void reset(long oldMsgSerial) { |
There was a problem hiding this comment.
I think reset() should take an ErrorInfo argument and pass that to the nack, and then the specific error is determined by whoever calls reset(). There will be different situations that will require different error messages/codes.
added 3 commits
October 6, 2016 15:13
This fixes part of #169
#170 had a problem where reeonnection from suspended and explicit channel recovery stops CompletionListener.onSuccess() being called for messages published after that. The problem was that the message serial in ConnectionManager and PendingMessageQueue got out of sync -- only the former was reset back to 0, so then PendingMessageQueue got confused by the message serial in an ack after a publish, and the callback was not called. Fixed by ensuring that msgSerial only gets reset when the connection id changes on connect, and then the PendingMessageQueue is reset at the same time. Also any pending messages (ones that have been sent but not acked) at that time are failed.
#170 One test for the recover case that was broken, and one test for the resume case that was probably not broken.
|
@paddy fixed your comments. |
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also protocol message logging.